Fix the build on OpenBSD #1333

oldlaptop · 2017-04-13T02:45:37Z

FSO doesn't build from source on OpenBSD anymore. This patch corrects the only significant problem: the iterator in LuaTable.h/cpp known as _L clashes with a #define in OpenBSD's standard headers (ctypes.h to be specific). The new name proposed here is simply taken from the type.

One other problem still prevents building on OpenBSD at this time: an assert in code/scripting/ade.h that calls std::is_trivially_copyable, available in libstdc++ >= 5 (where OpenBSD has 4.9 in ports/packages). OpenBSD on i386/amd64 will probably use clang/libc++ in the future, so this issue will go away anyhow. For the record, the following diff on top of this PR fixes the build on OpenBSD 6.1-current at this time:

diff --git a/code/scripting/ade.h b/code/scripting/ade.h
index 8478ea3ea..04cf920ef 100644
--- a/code/scripting/ade.h
+++ b/code/scripting/ade.h
@@ -226,7 +226,7 @@ template<class StoreType>
 class ade_obj: public ade_lib_handle {
  public:
        // Make sure that the stored type is compatible with our requirements
-       static_assert(std::is_trivially_copyable<StoreType>::value, "ADE object types must be trivially copyable!");
+       //static_assert(std::is_trivially_copyable<StoreType>::value, "ADE object types must be trivially copyable!");
 
        ade_obj(const char* in_name, const char* in_desc, const ade_lib_handle* in_deriv = NULL) {
                ade_table_entry ate;

asarium · 2017-04-13T07:48:12Z

code/scripting/lua/LuaTable.h

@@ -19,7 +19,7 @@ class LuaTable;
 * be in the exact same state when you call the next method of this class.
 */
 class LuaTableIterator {
-	lua_State* _L = nullptr;
+	lua_State* L_State = nullptr;


The rest of the class uses the format _varname so this field should do the same. _luaState would be an acceptable name.

Ah. I was reluctant to do that originally because leading underscores are theoretically reserved for the standard headers...

Only global names beginning with an underscore are reserved. For names in non-global namespaces (e.g. a class) the name may begin with an underscore but the next letter must be lower case which also explains why _L is an invalid name.

asarium · 2017-04-13T07:50:06Z

The static assert is needed to ensure that no runtime errors occur but it could be conditionally enabled only if support for is_trivially_copyable is detected. This could be done using a CMake platform check. See platformChecks.cmake for examples on how that is done.

The iterator known as _L, besides having an absurdly poor name, clashes with standard headers on OpenBSD -current at the time of writing. This commit is the minimum needed to build on OpenBSD with an updated C++ standard library.

oldlaptop · 2017-04-14T03:08:45Z

I've just added a platform check for is_trivially_copyable, can split off into another PR if desired.

asarium · 2017-04-14T07:51:29Z

cmake/platformChecks.cmake

@@ -16,6 +17,9 @@ CHECK_TYPE_SIZE("max_align_t" MAX_ALIGN_T)

 set(CMAKE_EXTRA_INCLUDE_FILES "cstddef")
 CHECK_TYPE_SIZE("std::max_align_t" STD_MAX_ALIGN_T LANGUAGE CXX)
+set(CMAKE_EXTRA_INCLUDE_FILES "type_traits")
+CHECK_TYPE_SIZE("std::is_trivially_copyable<int>" STD_IS_TRIVIALLY_COPYABLE LANGUAGE CXX)
+MESSAGE(${HAVE_STD_IS_TRIVIALLY_COPYABLE})


This seems to be working properly, although our Travis GCC config fails this test for some reason although it compiled the previous version properly. That doesn't matter though since Clang works properly.

Once you remove this test message, this is ready to be merged.

Ah, dammit. Thanks for catching... :)

My local setup passes the test, by the way (6.3.0 in Debian testing). Unfortunately my machines all have either 'old' gcc (4.9) or new gcc (>6) at the moment; I'll try to investigate behavior on gcc 5 (Debian packages 5.4 at the moment, in testing).

The reason gcc5 fails the test appears to be quite simple and stupid - from CMakeError.txt:

/usr/include/c++/5/bits/c++0x_warning.h:32:2: error: #error This file requires compiler and library support for the ISO C++ 2011 standard. This support must be enabled with the -std=c++11 or -std=gnu++11 compiler options.

I should be able to push a fix shortly.

asarium · 2017-04-15T08:57:10Z

cmake/platformChecks.cmake

@@ -16,6 +17,10 @@ CHECK_TYPE_SIZE("max_align_t" MAX_ALIGN_T)

 set(CMAKE_EXTRA_INCLUDE_FILES "cstddef")
 CHECK_TYPE_SIZE("std::max_align_t" STD_MAX_ALIGN_T LANGUAGE CXX)
+set(CMAKE_EXTRA_INCLUDE_FILES "type_traits")
+set(CMAKE_REQUIRED_FLAGS "-std=c++11") # required for g++ <= 5


Since this is only relevant for GCC it should be surrounded by a check for that compiler. if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU") should work since that is the same string our toolchain detection uses.

Since this applies to all the C++ checks, could you insert that code above the other CHECK_TYPE_SIZE? That will make the results more consistent.

asarium · 2017-04-15T21:15:24Z

cmake/platformChecks.cmake

-CHECK_TYPE_SIZE("std::max_align_t" STD_MAX_ALIGN_T LANGUAGE CXX)
-set(CMAKE_EXTRA_INCLUDE_FILES)
+if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
+    set(CMAKE_REQUIRED_FLAGS "-std=c++11") # required for g++ <= 5


That's not what I meant. Only this line should be in the if body. The other lines are required on all platforms but -std=c++11 is only relevant for GCC.

This is how it should look like:

if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU") set(CMAKE_REQUIRED_FLAGS "-std=c++11") # required for g++ <= 5 endif() set(CMAKE_EXTRA_INCLUDE_FILES "cstddef") CHECK_TYPE_SIZE("std::max_align_t" STD_MAX_ALIGN_T LANGUAGE CXX) set(CMAKE_EXTRA_INCLUDE_FILES "type_traits") CHECK_TYPE_SIZE("std::is_trivially_copyable<int>" STD_IS_TRIVIALLY_COPYABLE LANGUAGE CXX) set(CMAKE_EXTRA_INCLUDE_FILES) set(CMAKE_REQUIRED_FLAGS)

Yes, the template instantiation appears necessary for cmake to detect the type. If there's a better type for it than int, I'm open to suggestions.

asarium · 2017-04-16T08:34:49Z

Yes, that looks good now.

asarium requested changes Apr 13, 2017

View reviewed changes

asarium added this to the Release 3.8 milestone Apr 13, 2017

asarium added the fix A fix for bugs, not-a-bugs, and/or regressions. label Apr 13, 2017

oldlaptop force-pushed the openbsd branch from cbf124a to fb80c12 Compare April 13, 2017 12:43

s/_L/L_State

8f76537

The iterator known as _L, besides having an absurdly poor name, clashes with standard headers on OpenBSD -current at the time of writing. This commit is the minimum needed to build on OpenBSD with an updated C++ standard library.

oldlaptop force-pushed the openbsd branch from fb80c12 to 8f76537 Compare April 13, 2017 13:07

oldlaptop changed the title ~~(Mostly) fix the build on OpenBSD~~ Fix the build on OpenBSD Apr 14, 2017

asarium requested changes Apr 14, 2017

View reviewed changes

oldlaptop force-pushed the openbsd branch 2 times, most recently from 35d8427 to b8f1d76 Compare April 15, 2017 03:01

asarium requested changes Apr 15, 2017

View reviewed changes

oldlaptop force-pushed the openbsd branch from b8f1d76 to 12d45c2 Compare April 15, 2017 21:11

asarium requested changes Apr 15, 2017

View reviewed changes

add platform check for std::is_trivially_copyable

4b36e3c

Yes, the template instantiation appears necessary for cmake to detect the type. If there's a better type for it than int, I'm open to suggestions.

oldlaptop force-pushed the openbsd branch from 12d45c2 to 4b36e3c Compare April 16, 2017 03:53

asarium approved these changes Apr 16, 2017

View reviewed changes

asarium merged commit 8850efc into scp-fs2open:master Apr 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the build on OpenBSD #1333

Fix the build on OpenBSD #1333

oldlaptop commented Apr 13, 2017

asarium Apr 13, 2017

oldlaptop Apr 13, 2017

asarium Apr 13, 2017

asarium commented Apr 13, 2017

oldlaptop commented Apr 14, 2017

asarium Apr 14, 2017

oldlaptop Apr 15, 2017

oldlaptop Apr 15, 2017

oldlaptop Apr 15, 2017 •

edited

asarium Apr 15, 2017

oldlaptop Apr 15, 2017

asarium Apr 15, 2017

oldlaptop Apr 16, 2017

asarium commented Apr 16, 2017

Fix the build on OpenBSD #1333

Fix the build on OpenBSD #1333

Conversation

oldlaptop commented Apr 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asarium commented Apr 13, 2017

oldlaptop commented Apr 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oldlaptop Apr 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asarium commented Apr 16, 2017

oldlaptop Apr 15, 2017 •

edited